Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying alignment for functions #81234

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Conversation

repnop
Copy link
Contributor

@repnop repnop commented Jan 21, 2021

Fixes #75072

This allows the user to specify alignment for functions, which can be useful for low level work where functions need to necessarily be aligned to a specific value.

I believe the error cases not covered in the match are caught earlier based on my testing so I had them just return None.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2021
@repnop
Copy link
Contributor Author

repnop commented Jan 21, 2021

Not sure how that submodule got added, but removed 🤔

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Jan 21, 2021

nominating for t-lang for now, I expect us to land this unstably first if we want to do so, but it does seem desirable to do so imo.

@lcnr lcnr added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 21, 2021
@joshtriplett
Copy link
Member

joshtriplett commented Feb 9, 2021

This seems reasonable to me, to add on an unstable basis. (There needs to be a feature-gate added.)

I also think many codebases will want to set this for all functions using a compiler option (e.g. set in a Cargo profile). But supporting it on individual functions seems reasonable as well.

EDIT: We discussed this in today's lang-team meeting, and this was the general consensus:

  • Please add a feature-gate (e.g. func_align)
  • When this is merged, we'll need a tracking issue

@repnop
Copy link
Contributor Author

repnop commented Feb 9, 2021

Awesome! I should hopefully have some time soon to do that :)

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting, and we're in favor of adding this to nightly, for experimentation, as soon as there's a feature-gate added.

@rfcbot fcp merge
@rfcbot concern feature-gate

@rfcbot
Copy link

rfcbot commented Feb 16, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 16, 2021
@repnop repnop force-pushed the fn-alignment branch 2 times, most recently from 383465c to dd575a9 Compare February 17, 2021 19:41
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@repnop
Copy link
Contributor Author

repnop commented Feb 17, 2021

I think I added all the requests, any other changes that need made?

@joshtriplett
Copy link
Member

@rfcbot resolve feature-gate

Thank you!

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 23, 2021
@rfcbot
Copy link

rfcbot commented Feb 23, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented Feb 23, 2021

☔ The latest upstream changes (presumably #82443) made this pull request unmergeable. Please resolve the merge conflicts.

@Havvy
Copy link
Contributor

Havvy commented Mar 4, 2021

Shouldn't this be going through the RFC process?

@lcnr
Copy link
Contributor

lcnr commented Apr 5, 2021

if ci passes, please squash your commits

after that r=me

@bors delegate+

@bors
Copy link
Contributor

bors commented Apr 5, 2021

✌️ @repnop can now approve this pull request

@repnop
Copy link
Contributor Author

repnop commented Apr 5, 2021

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Apr 5, 2021

📌 Commit 448d076 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021
@bors
Copy link
Contributor

bors commented Apr 6, 2021

⌛ Testing commit 448d076 with merge a6e7a5a...

@bors
Copy link
Contributor

bors commented Apr 6, 2021

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing a6e7a5a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2021
@bors bors merged commit a6e7a5a into rust-lang:master Apr 6, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 6, 2021
@bors bors mentioned this pull request Apr 6, 2021
@RalfJung
Copy link
Member

RalfJung commented Apr 15, 2021

What are the language-level effects of this? Is this mainly a hint for how to put the compiled binary code together, or is his somehow observable inside Rust (e.g. by adding new kinds of UB)?

@repnop
Copy link
Contributor Author

repnop commented Apr 15, 2021

Is this mainly a hint for how to put the compiled binary code together

yes, its not intended to introduce any visible effects from within Rust, but to allow telling LLVM that the function needs to be aligned to a specific boundary for situations in which that's necessary and you can't rely on the native alignment placing it at an appropriate address (e.g. so it can be inserted into a hardware register, in my case)

by adding new kinds of UB

I think the only thing that may have been glossed over in this PR as it stands right now is being able to under-align functions, which I'll need to investigate to see if we need to add some additional checks to the #[repr(align(...))] as to not allow that to happen, or if LLVM will ignore the alignment hint and force it up to the native alignment since it would fulfill the request anyways (which seems like it may be the case? at least with some limited test in the playground)

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2021
…ulacrum

[beta] Bootstrap from stable

This is the follow up to master/beta promotion, as well as the first round of backports:

* Revert "Allow specifying alignment for functions rust-lang#81234"
* Revert rust-lang#85176 addition of clone_from for ManuallyDrop rust-lang#85758
* rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType rust-lang#84867
*  [beta] Update cargo rust-lang#86563

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying function alignment